Skip to content

feat: prevent core modules from getting loaded multiple times #1196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Mar 13, 2018
Merged

feat: prevent core modules from getting loaded multiple times #1196

merged 4 commits into from
Mar 13, 2018

Conversation

sebawita
Copy link
Contributor

@sebawita sebawita commented Feb 9, 2018 •
edited
Loading

This change should prevent NativeScriptModule and NativeScriptAnimationsModule from getting loaded in multiple NgModules across an Angular NativeScript app.

When the core modules are imported multiple times, this change will cause Errors to be thrown with the following message:

  • For NativeScriptModule:

NativeScriptModule has already been loaded. Import NativeScriptModule in the AppModule only.

  • For NativeScriptAnimationsModule:

NativeScriptAnimationsModule has already been loaded. Import NativeScriptAnimationsModule in the AppModule only.

BREAKING CHANGE: importing NativeScriptModule and NativeScriptAnimationsModule in multiple ngModules is no longer allowed.

To migrate:

  • in AppModule:
    • import NativeScriptModule
    • importNativeScriptAnimationsModule - only if you are planning to use Angular Animations
  • in the remaining modules:
    • remove NativeScriptModule imports and replace with NativeScriptCommonModule import
    • remove NativeScriptAnimationsModule imports

Example

Before:
app.module.ts:

import { NativeScriptModule } from 'nativescript-angular/nativescript.module';
import { NativeScriptAnimationsModule } from 'nativescript-angular/animations';
...
@NgModule({
  imports: [
    NativeScriptModule,
    NativeScriptAnimationsModule
  ],
...
})

another.module.ts:

import { NativeScriptModule } from 'nativescript-angular/nativescript.module';
import { NativeScriptAnimationsModule } from 'nativescript-angular/animations';
...
@NgModule({
  imports: [
    NativeScriptModule,
    NativeScriptAnimationsModule
  ],
...
})

After:
app.module.ts:

import { NativeScriptModule } from 'nativescript-angular/nativescript.module';
import { NativeScriptAnimationsModule } from 'nativescript-angular/animations';
...
@NgModule({
  imports: [
    NativeScriptModule,
    NativeScriptAnimationsModule
  ],
...
})

another.module.ts:

import { NativeScriptCommonModule } from 'nativescript-angular/common';
...
@NgModule({
  imports: [
    NativeScriptCommonModule
  ],
...
})

@ghost ghost assigned sebawita Feb 9, 2018
@ghost ghost added the in progress label Feb 9, 2018
@vchimev
Copy link
Contributor

vchimev commented Feb 9, 2018

There are failing tests in the e2e-renderer-android suite:

should navigate to page.Action Bar scenario dynamically add/remove ActionItems should navigate to page
should find elements.Action Bar scenario dynamically add/remove ActionItems should find elements
should initially render the action items in the correct order.Action Bar scenario dynamically add/remove ActionItems should initially render the action items in the correct order
should detach first element when its condition is false.Action Bar scenario dynamically add/remove ActionItems should detach first element when its condition is false
should attach first element in the correct position.Action Bar scenario dynamically add/remove ActionItems should attach first element in the correct position
should detach second element when its condition is false.Action Bar scenario dynamically add/remove ActionItems should detach second element when its condition is false
should attach second element in the correct position.Action Bar scenario dynamically add/remove ActionItems should attach second element in the correct position
should detach and then reattach both at correct places.Action Bar scenario dynamically add/remove ActionItems should detach and then reattach both at correct places

@vchimev
Copy link
Contributor

vchimev commented Feb 9, 2018

There is a failing test in the e2e-router-ios suite:

should navigate to lazy/home.Navigate to lazy module should navigate to lazy/home

sis0k0
sis0k0 suggested changes Feb 12, 2018
View reviewed changes
Copy link
Contributor

@sis0k0 sis0k0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the PR name to follow this convention: https://docs.google.com/document/d/1QrDFcIiPjSLDn3EL15IJygNPiHORgU1_OOAqWjiDU5Y/edit#heading=h.uyo6cb12dt6w? Also it may be good to add a 'BREAKING CHANGES' section in the footer (see the last section of the above doc).

@sis0k0 sis0k0 added this to the 6.0 milestone Feb 12, 2018
@sebawita sebawita changed the title Prevent core modules from getting loaded multiple times feat: prevent core modules from getting loaded multiple times Feb 12, 2018
@sebawita
Copy link
Contributor Author

I've made the requested changes

sis0k0
sis0k0 approved these changes Feb 12, 2018
View reviewed changes
Copy link
Contributor

@sis0k0 sis0k0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after green build.

@ghost ghost assigned sis0k0 Mar 12, 2018
@sis0k0 sis0k0 merged commit 010fed7 into NativeScript:master Mar 13, 2018
@ghost ghost removed the in progress label Mar 13, 2018
sis0k0 added a commit to ProgressNS/nativescript-ui-samples-angular that referenced this pull request Mar 13, 2018
@sis0k0
refactor: replace NativeScriptModule with NativeScriptCommonModule
80f706b 
imports

NativeScriptModule should be imported only in the root NgModule. For all
other feature modules, NativeScriptCommonModule should be used instead.
Check out: NativeScript/nativescript-angular#1196
sis0k0 added a commit to ProgressNS/nativescript-ui-samples-angular that referenced this pull request Mar 13, 2018
@sis0k0
refactor: replace NativeScriptModule with NativeScriptCommonModule
32b0c0b 
NativeScriptModule should be imported only in the root NgModule. For all
other feature modules, NativeScriptCommonModule should be used instead.
Check out NativeScript/nativescript-angular#1196.
@sis0k0 sis0k0 mentioned this pull request Mar 13, 2018
Merged
sis0k0 added a commit to NativeScript/sample-Groceries that referenced this pull request Mar 13, 2018
@sis0k0
refactor: replace NativeScriptModule with NativeScriptCommonModule
0ccdeab 
NativeScriptModule should be imported only in the root NgModule. For all
other feature modules, NativeScriptCommonModule should be used instead.
Check out NativeScript/nativescript-angular#1196.
@sis0k0 sis0k0 mentioned this pull request Mar 13, 2018
Merged
sis0k0 added a commit to NativeScript/sample-Groceries that referenced this pull request Mar 14, 2018
@sis0k0
refactor: replace NativeScriptModule with NativeScriptCommonModule (#300
a32b087 
)

NativeScriptModule should be imported only in the root NgModule. For all
other feature modules, NativeScriptCommonModule should be used instead.
Check out NativeScript/nativescript-angular#1196.
sis0k0 added a commit to ProgressNS/nativescript-ui-samples-angular that referenced this pull request Mar 14, 2018
@sis0k0
refactor: replace NativeScriptModule with NativeScriptCommonModule (#97)
243f49e 
NativeScriptModule should be imported only in the root NgModule. For all
other feature modules, NativeScriptCommonModule should be used instead.
Check out NativeScript/nativescript-angular#1196.
sis0k0 added a commit to NativeScript/nativescript-schematics that referenced this pull request Mar 26, 2018
@sis0k0
feat: add schematic for fixing module imports in feature modules
068b752 
Replaces NativeScriptModule imports with NativeScriptCommonModule
imports in feature modules.
Removes NativeScriptAnimationsModule imports from feature modules and
adds import to that module in the root module.
Covers the breaking change NativeScript/nativescript-angular#1196
@sis0k0 sis0k0 mentioned this pull request Mar 26, 2018
Merged
sis0k0 added a commit to NativeScript/nativescript-schematics that referenced this pull request Mar 28, 2018
@sis0k0
feat: add schematic for fixing module imports in feature modules
180d27a 
Replaces NativeScriptModule imports with NativeScriptCommonModule
imports in feature modules.
Removes NativeScriptAnimationsModule imports from feature modules and
adds import to that module in the root module.
Covers the breaking change NativeScript/nativescript-angular#1196
manoldonev pushed a commit to NativeScript/nativescript-app-templates that referenced this pull request Jan 9, 2019
@sis0k0 @manoldonev
refactor: replace NativeScriptModule with NativeScriptCommonModule (#19)
e003241 
NativeScriptModule should be imported only in the root NgModule. For all
other feature modules, NativeScriptCommonModule should be used instead.
Check out NativeScript/nativescript-angular#1196.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sis0k0 sis0k0 sis0k0 approved these changes

Assignees

@sebawita sebawita

@sis0k0 sis0k0

Labels
None yet
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@sebawita @vchimev @sis0k0